chore: ps example. #327
chore: ps example. #327
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
Summary of ChangesHello @k82cn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, comprehensive example showcasing a Parameter Server architecture for distributed machine learning, built upon the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a parameter server example, which is a great addition. It also refactors the working_directory attribute for applications to be optional, providing a more sensible default when it's not specified. Additionally, there are some nice improvements to the Python SDK's package installation logic.
My review includes a few suggestions to improve the new example code's robustness, such as handling file paths correctly and ensuring variables are initialized before use. I also found a small improvement for file handling in the Python SDK. Overall, the changes are well-done.
| accuracy = evaluate(model, test_loader) | ||
| print("Iter {}: \taccuracy is {:.1f}".format(i, accuracy)) | ||
|
|
||
| print("Final accuracy is {:.1f}.".format(accuracy)) |
There was a problem hiding this comment.
The accuracy variable is only defined inside the if i % 10 == 0: block within the loop. If the loop range were smaller (e.g., range(0)), this line would raise an UnboundLocalError. It would be safer to initialize accuracy to a default value (e.g., 0.0) before the loop.
| with FileLock(os.path.expanduser("~/data.lock")): | ||
| train_loader = torch.utils.data.DataLoader( | ||
| datasets.MNIST( | ||
| "~/data", train=True, download=True, transform=mnist_transforms | ||
| ), | ||
| batch_size=128, | ||
| shuffle=True, | ||
| ) | ||
| test_loader = torch.utils.data.DataLoader( | ||
| datasets.MNIST("~/data", train=False, transform=mnist_transforms), | ||
| batch_size=128, | ||
| shuffle=True, | ||
| ) |
There was a problem hiding this comment.
The path ~/data is not guaranteed to be expanded by torchvision.datasets.MNIST. It's better to use os.path.expanduser explicitly to ensure the path is resolved correctly. Also, it's good practice to place the lock file within the directory it's protecting to keep things organized. I've refactored this part to define data_dir once and use it for both the dataset and the lock file.
| with FileLock(os.path.expanduser("~/data.lock")): | |
| train_loader = torch.utils.data.DataLoader( | |
| datasets.MNIST( | |
| "~/data", train=True, download=True, transform=mnist_transforms | |
| ), | |
| batch_size=128, | |
| shuffle=True, | |
| ) | |
| test_loader = torch.utils.data.DataLoader( | |
| datasets.MNIST("~/data", train=False, transform=mnist_transforms), | |
| batch_size=128, | |
| shuffle=True, | |
| ) | |
| data_dir = os.path.expanduser("~/data") | |
| # We add FileLock here because multiple workers will want to | |
| # download data, and this may cause overwrites since | |
| # DataLoader is not threadsafe. | |
| with FileLock(os.path.join(data_dir, ".lock")): | |
| train_loader = torch.utils.data.DataLoader( | |
| datasets.MNIST( | |
| data_dir, train=True, download=True, transform=mnist_transforms | |
| ), | |
| batch_size=128, | |
| shuffle=True, | |
| ) | |
| test_loader = torch.utils.data.DataLoader( | |
| datasets.MNIST(data_dir, train=False, transform=mnist_transforms), | |
| batch_size=128, | |
| shuffle=True, | |
| ) |
| # Check for pyproject.toml or setup.py | ||
| if "pyproject.toml" in contents: | ||
| pyproject_path = os.path.join(install_path, "pyproject.toml") | ||
| with open(pyproject_path, "r") as f: |
There was a problem hiding this comment.
It's a good practice to explicitly specify the file encoding when opening text files. This avoids potential UnicodeDecodeError if the file contains non-ASCII characters and the system's default encoding is not UTF-8. Please add encoding="utf-8".
| with open(pyproject_path, "r") as f: | |
| with open(pyproject_path, "r", encoding="utf-8") as f: |
Signed-off-by: Klaus Ma <klausm@nvidia.com>
Signed-off-by: Klaus Ma <klausm@nvidia.com>
1 participant
No description provided.